waveshare panel driver update #6499

waveshare · 2024-11-29T04:05:47Z

add waveshare 13.3 inch panel support
add 4lane support

pelwell · 2024-11-29T11:52:18Z

arch/arm/boot/dts/overlays/README

        10_1_inch               10.1" 1280x800
        11_9_inch               11.9" 320x1480
+	13.3inch-30fps        13.3″ 1920x1080 2lane
+	13.3inch-60fps        13.3″ 1920x1080 4lane


Replace the TABs with spaces, fix the indentation so it all lines up, and get the parameter names right!

pelwell · 2024-11-29T11:54:06Z

arch/arm/boot/dts/overlays/vc4-kms-dsi-waveshare-panel-overlay.dts

 				   <&touch>, "touchscreen-inverted-y?";
 		8_8_inch = <&panel>, "compatible=waveshare,8.8inch-panel";
+		13_3_60fps_inch = <&panel>, "compatible=waveshare,13.3inch-60fps-panel";
+		13_3_30fps_inch = <&panel>, "compatible=waveshare,13.3inch-30fps-panel";


These names look wrong - 13_3_inch_60fps and 13_3_inch_30fps make more sense.

Yeah, I found the problem. I'm working on it.
I think it would be more appropriate to read 13_3_inch_4lane.

waveshare · 2024-11-29T12:37:54Z

Wait a minute. It looks like some of the files haven't been updated yet.

waveshare · 2024-11-29T12:57:32Z

Changes have been completed. PTAL.

pelwell · 2024-11-30T14:20:20Z

Fix the tabs again, then all the checkpatch errors and warnings (all of them look reasonable) and squash back down to 3 commits - driver, config and DT.

6by9

Is there a need for this new touch driver?

There is already https://github.com/torvalds/linux/blob/master/drivers/input/touchscreen/ilitek_ts_i2c.c which claims to be for the Ilitek 23XX, 25XX, and Lego chips. Which chip are you actually using on this display, and is it compatible?

6by9 · 2024-12-02T13:30:13Z

drivers/input/touchscreen/ilitek_2511/ilitek_platform_init.c

+module_i2c_driver(ilitek_touch_device_driver);
+
+MODULE_AUTHOR("waveshare");
+MODULE_DESCRIPTION("WS_xinchDSI_Touch Driver");


Really? The copyright headers all list ILI Technology Corporation and Luca Hsu.

I'll check with my coworker that this TP driver came directly to us from our supplier

6by9 · 2024-12-02T13:32:43Z

drivers/input/touchscreen/ilitek_2511/ilitek_platform_init.c

+MODULE_DEVICE_TABLE(i2c, tpd_device_id);
+
+const struct of_device_id touch_dt_match_table[] = {
+	{.compatible = "tchip,ilitek2" },


Who is tchip as a manufacturer? It's not defined in https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/vendor-prefixes.yaml

6by9 · 2024-12-02T15:13:08Z

I've just noticed that we already have an ilitek251x overlay in the tree using the ili210x driver that lists an ilitek,ili251x compatible.

I'd hope either ili210x or ilitek_ts_i2c would support your touch controller.

6by9

Thank you for the update - using the mainline drivers is always preferable to adding yet another downstream one.

I've made comment on one coding style issue that checkpatch will complain over. This is the sort of patch that does want to be sent upstream, so we will be quite strict over it. CI has now run checkpatch for this PR, so please fix the errors.

6by9 · 2024-12-03T14:16:25Z

drivers/input/touchscreen/ili210x.c

+			return error;
+		}
+	}
+	else{


Space before the {

6by9 · 2024-12-03T14:16:57Z

drivers/input/touchscreen/ili210x.c

+	else{
+		timer_setup(&priv->poll_timer, ili210x_poll_timer_callback, 0);
+		mod_timer(&priv->poll_timer, jiffies + msecs_to_jiffies(ILI2XXX_POLL_PERIOD));
+		INIT_WORK(&priv->poll_work, ili210x_poll_work);


You don't cancel the workqueue or delete the timer on remove.
See 818bb6d for doing the same on edt-ft5x06(*).

In this case it probably wants to go into ili210x_stop though as they've avoided adding a remove function.

(*) Looking at it, I think the cancel_work_sync should come first.
Comparing to tc358743 which also has a polling option, that uses slightly different semantics again - there never is one clear option, and it needs careful thought over what triggers what.

Thank you very much for your valuable guidance on maintaining coding standards, especially for patches intended for upstream submission.I will promptly address the issues identified by checkpatch and correct the coding style problem you pointed out. Once the updates are complete, I will submit the revised version for further review.

Signed-off-by: eng33 <[email protected]>

waveshare · 2024-12-05T09:37:33Z

Thank you for your feedback and guidance, the changes are as above, please feel free to let me know if further improvements are needed or additional testing is recommended.
I'm happy to make adjustments to ensure the patch meets upstream standards.

Signed-off-by: eng33 <[email protected]>

pelwell · 2024-12-05T10:14:14Z

You keep repeating the same mistakes. Download overlaycheck from our utils repo (https://github.com/raspberrypi/utils/tree/master/overlaycheck) to get it right before submitting.

waveshare · 2024-12-05T10:23:06Z

You keep repeating the same mistakes. Download overlaycheck from our utils repo (https://github.com/raspberrypi/utils/tree/master/overlaycheck) to get it right before submitting.

Ok, we will pay attention to this problem in the future

waveshare · 2024-12-05T10:50:29Z

Already passed overlaycheck.

pelwell · 2024-12-05T10:59:45Z

Yes, this one is looking OK.

6by9 · 2024-12-05T12:17:52Z

Looks reasonable to me.

I'm not sure which order the ili210x_stop devm_action and the remove hook will be called in, and that could cause issues. Testing with sudo rmmod ili210x to confirm it doesn't crash, or better still adding a debug line in remove confirms that stop has already been set would be nice to know it's OK. However unloading modules is a rare enough thing that I'm not going to fuss.

waveshare · 2024-12-07T07:20:15Z

Looks reasonable to me.

I'm not sure which order the ili210x_stop devm_action and the remove hook will be called in, and that could cause issues. Testing with sudo rmmod ili210x to confirm it doesn't crash, or better still adding a debug line in remove confirms that stop has already been set would be nice to know it's OK. However unloading modules is a rare enough thing that I'm not going to fuss.

Thank you for your suggestions! I tested unloading the module using sudo rmmod ili210x, and there were no crashes during the process. I also added a debug line in the remove function to confirm that stop had already been set. All tests ran smoothly without any issues.

I agree that unloading modules is not a common operation, so it’s unlikely to be a major concern in practice. Still, it’s good to have verified this just in case.

Thank you all again for taking the time to share your suggestions and insights!

kernel: Revert "PCI: Warn if no host bridge NUMA node info" See: raspberrypi/linux#6531 kernel: drm/v3d: Enable Performance Counters before clearing them See: raspberrypi/linux#6514 kernel: configs: Enable Intel Wireless (PCI) drivers See: raspberrypi/linux#6526 kernel: media: i2c: ov9282: Correct the exposure offset See: raspberrypi/linux#6522 kernel: waveshare panel driver update See: raspberrypi/linux#6499 kernel: dtoverlays: Add Arducam override for ov9281 See: raspberrypi/linux#6506 kernel: nvme-pci: Disable Host Memory Buffer usage See: raspberrypi/linux#6510 kernel: Allow setting I²C clock frequency via i2c_arm_baudrate dtparam when using pimidi overlay See: raspberrypi/linux#6513

pelwell reviewed Nov 29, 2024

View reviewed changes

waveshare force-pushed the 13.3INCH branch from 8325f51 to c56fb61 Compare November 29, 2024 12:18

waveshare force-pushed the 13.3INCH branch 3 times, most recently from ddbe278 to 3a3cd0c Compare November 29, 2024 12:55

waveshare force-pushed the 13.3INCH branch from e91e47e to 708c7c9 Compare December 2, 2024 13:34

6by9 reviewed Dec 2, 2024

View reviewed changes

waveshare force-pushed the 13.3INCH branch from 708c7c9 to 0cba6d5 Compare December 3, 2024 14:10

6by9 reviewed Dec 3, 2024

View reviewed changes

eng33 added 2 commits December 5, 2024 17:19

drivers:input:touchscreen: Add support for no irq to ili210x driver

7a6cb0f

Signed-off-by: eng33 <[email protected]>

drivers:gpu:drm:panel: Added waveshare 13.3inch panel(support 2/4lane)

c17aa54

Signed-off-by: eng33 <[email protected]>

waveshare force-pushed the 13.3INCH branch 2 times, most recently from 4ef1f88 to c47be70 Compare December 5, 2024 09:26

arch:arm:boot:dts:overlays: Added waveshare 13.3inch panel support

b8730f7

Signed-off-by: eng33 <[email protected]>

waveshare force-pushed the 13.3INCH branch from c47be70 to b8730f7 Compare December 5, 2024 10:14

pelwell merged commit e442e5c into raspberrypi:rpi-6.6.y Dec 6, 2024
11 of 12 checks passed

waveshare deleted the 13.3INCH branch June 24, 2025 03:42

waveshare panel driver update #6499

waveshare panel driver update #6499

Uh oh!

Conversation

waveshare commented Nov 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waveshare commented Nov 29, 2024

Uh oh!

waveshare commented Nov 29, 2024

Uh oh!

pelwell commented Nov 30, 2024

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6by9 commented Dec 2, 2024

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waveshare commented Dec 5, 2024

Uh oh!

pelwell commented Dec 5, 2024

Uh oh!

waveshare commented Dec 5, 2024

Uh oh!

waveshare commented Dec 5, 2024

Uh oh!

pelwell commented Dec 5, 2024

Uh oh!

6by9 commented Dec 5, 2024

Uh oh!

Uh oh!

waveshare commented Dec 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants